-
-
Notifications
You must be signed in to change notification settings - Fork 6.9k
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Attach behavior via "as behaviorName" with Closure #20248
Attach behavior via "as behaviorName" with Closure #20248
Conversation
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## master #20248 +/- ##
============================================
- Coverage 64.95% 64.95% -0.01%
- Complexity 11395 11396 +1
============================================
Files 430 430
Lines 36923 36925 +2
============================================
- Hits 23985 23984 -1
- Misses 12938 12941 +3 ☔ View full report in Codecov by Sentry. |
$this->assertSame($behavior, $component->detachBehavior('a')); | ||
$this->assertFalse($component->hasProperty('p')); | ||
$this->expectException('yii\base\UnknownMethodException'); | ||
$component->test(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
The test was passing but the assertions following this were never run.
@@ -376,6 +380,9 @@ public function testDetachBehavior() | |||
|
|||
$detachedBehavior = $component->detachBehavior('z'); | |||
$this->assertNull($detachedBehavior); | |||
|
|||
$this->expectException('yii\base\UnknownMethodException'); | |||
$component->test(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This made more sense to test here than in attachBehaviors
where it lived and was causing the test to end prematurely.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Added lines #L193 - L194 were not covered by tests
Could you add a test for that.
@schmunk42 the test for that is here. I'm not sure why that warning is showing in the Github diff. It isn't showing here: https://app.codecov.io/gh/yiisoft/yii2/pull/20248 |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you rollback changes to the two files under db/
?
This reverts commit ff60985.
@machour Done. CS/lint check will fail, but not from changes in this PR. |
@timkelty thanks for your time and efforts. Can you please resolve the conflict to enable final review and possibly merging it? |
# Conflicts: # framework/base/Component.php # tests/framework/base/ComponentTest.php
All set, @mtangoo. |
Thanks for your time and efforts. We will review and merge it if all is good without delay! |
@timkelty the PR is good. Can you add a line in changelog so that we can merge this? |
Done! |
@timkelty thanks for your time and efforts |
Allows behaviors attached with the
'as behaviorName'
syntax to use a Closure.In the process of writing a test, I discovered the
\yiiunit\framework\base\ComponentTest::testAttachBehavior
test was broken (it has assertions that were never reached), so I fixed that as well.